Skip to content

[SPARK-9054] [SQL] Rename RowOrdering to InterpretedOrdering; use newOrdering in SMJ#7973

Closed
JoshRosen wants to merge 3 commits into
apache:masterfrom
JoshRosen:SPARK-9054
Closed

[SPARK-9054] [SQL] Rename RowOrdering to InterpretedOrdering; use newOrdering in SMJ#7973
JoshRosen wants to merge 3 commits into
apache:masterfrom
JoshRosen:SPARK-9054

Conversation

@JoshRosen

@JoshRosen JoshRosen commented Aug 5, 2015

Copy link
Copy Markdown
Contributor

This patches renames RowOrdering to InterpretedOrdering and updates SortMergeJoin to use the SparkPlan methods for constructing its ordering so that it may benefit from codegen.

This is an updated version of #7408.

@JoshRosen

JoshRosen commented Aug 5, 2015

Copy link
Copy Markdown
Contributor Author

/cc @rxin @yhuai, this is needed to fix an issue that I found in my SMJ patch.

@JoshRosen JoshRosen mentioned this pull request Aug 5, 2015
Closed
yhuai
yhuai reviewed Aug 5, 2015
View reviewed changes
Comment thread sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeJoin.scala

@yhuai yhuai Aug 5, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we at a comment at https://github.com/apache/spark/pull/7973/files#diff-b669f8cf35f1d2d786582f4d8c49ed14R59 to explain the direction must be Ascending. So, the direction in requiredOrders is consistent with the direction we are using at here.

@JoshRosen JoshRosen Aug 5, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@JoshRosen JoshRosen Aug 5, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

yhuai
yhuai reviewed Aug 5, 2015
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ordering.scala

@yhuai yhuai Aug 5, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this name also reflect its using Ascending?

@yhuai yhuai Aug 5, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(super minor)

@JoshRosen JoshRosen Aug 5, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, it could. I only left it as-is out of a desire to minimize changes. Can fix later.

@yhuai yhuai Aug 5, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it is totally fine to leave it as-is.

@yhuai

yhuai commented Aug 5, 2015

Copy link
Copy Markdown
Contributor

LGTM

@SparkQA

SparkQA commented Aug 5, 2015

Copy link
Copy Markdown

Test build #39915 has finished for PR 7973 at commit 34b8e0c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Aug 5, 2015

Copy link
Copy Markdown

Test build #39918 has finished for PR 7973 at commit e610655.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen

JoshRosen commented Aug 5, 2015

Copy link
Copy Markdown
Contributor Author

This passed on an earlier run and only failed the latest run due to a known flaky test, so I'm going to merge this to master and branch-1.5 in order to unblock work on my other patch.

asfgit pushed a commit that referenced this pull request Aug 5, 2015
@JoshRosen
[SPARK-9054] [SQL] Rename RowOrdering to InterpretedOrdering; use new…
618dc63 
…Ordering in SMJ

This patches renames `RowOrdering` to `InterpretedOrdering` and updates SortMergeJoin to use the `SparkPlan` methods for constructing its ordering so that it may benefit from codegen.

This is an updated version of #7408.

Author: Josh Rosen <joshrosen@databricks.com>

Closes #7973 from JoshRosen/SPARK-9054 and squashes the following commits:

e610655 [Josh Rosen] Add comment RE: Ascending ordering
34b8e0c [Josh Rosen] Import ordering
be19a0f [Josh Rosen] [SPARK-9054] [SQL] Rename RowOrdering to InterpretedOrdering; use newOrdering in more places.

(cherry picked from commit 9c87892)
Signed-off-by: Josh Rosen <joshrosen@databricks.com>
@asfgit asfgit closed this in 9c87892 Aug 5, 2015
@JoshRosen JoshRosen deleted the SPARK-9054 branch August 29, 2016 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JoshRosen @yhuai @SparkQA